Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Added "id" parameter for make_forecaseting_frame so that it will not … #1080

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Added "id" parameter for make_forecaseting_frame so that it will not … #1080

wants to merge 1 commit into from

Conversation

heib6xinyu
Copy link

…have hard coded 'id' in the result.

This is my edition of make_forecasting_frame of the issue #1077 .
I added id parameter to the function, so the result will be (a, t1) instead of (id, t1) for item that has the id of a.

@nils-braun
Copy link
Collaborator

Hi @heib6xinyu - thanks for providing a PR!
As we discussed on the linked issue, the function make_forecasting_frame is only meant to be used for singular time series (as the name "frame" suggests). While having an id parameter is technically possible (as you have shown in your PR), it might make people think the function should be used for timeseries with multiple ids (because all other functions which allow for different id columns have a parameter to control the id column).
Therefore I would not like to add the id parameter. Also, it would be a breaking API change of our public API, so we need to be very careful about it.

Would it be possible for you to do the change from id to e.g. a as a post-processing step in your code?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@heib6xinyu @nils-braun